Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a simple version to fill the unique needs of slit4 #672

Closed
wants to merge 11 commits into from

Conversation

stan-dot
Copy link
Contributor

fixes #587

@stan-dot stan-dot added the enhancement New feature or request label Jul 15, 2024
@stan-dot stan-dot self-assigned this Jul 15, 2024
@stan-dot stan-dot linked an issue Jul 15, 2024 that may be closed by this pull request
Copy link
Contributor

@DiamondJoseph DiamondJoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try it out on the beamline tomorrow, the signals are definitely currently wrong.

src/dodal/devices/slits.py Outdated Show resolved Hide resolved
@callumforrester
Copy link
Contributor

For signal correctness, try running the system tests on the Diamond network: https://github.com/DiamondLightSource/dodal?tab=readme-ov-file#testing-connectivity

@stan-dot stan-dot marked this pull request as ready for review July 16, 2024 12:14
@stan-dot
Copy link
Contributor Author

connectivity check fails for this device AND panda1

@stan-dot stan-dot force-pushed the 587-create-device-for-i22-slits_4 branch from 2923a80 to c5d7ef2 Compare July 24, 2024 16:08
@stan-dot
Copy link
Contributor Author

will test on 30.07

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.44%. Comparing base (46132f0) to head (291050c).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #672   +/-   ##
=======================================
  Coverage   94.43%   94.44%           
=======================================
  Files         113      113           
  Lines        4581     4588    +7     
=======================================
+ Hits         4326     4333    +7     
  Misses        255      255           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stan-dot stan-dot force-pushed the 587-create-device-for-i22-slits_4 branch from c5d7ef2 to 74be14d Compare July 29, 2024 09:20
@stan-dot stan-dot force-pushed the 587-create-device-for-i22-slits_4 branch 2 times, most recently from 4ee837e to 82c11ef Compare July 30, 2024 17:01
@stan-dot
Copy link
Contributor Author

not sure what is the desired behavior, which PVs do we want to expose?

the EDM screen shows:
X, Y, HDSO, VDSO, of the 2 latter each has a raw and 'real' variant.
There is an abscence of words known from other slits, such as x centre, or gap

@DiamondJoseph please advise

@DiamondJoseph
Copy link
Contributor

Let's ask the beamline scientist tomorrow, but I assume that we want all of them.
Let's also keep them as Motors, same as the existing Slits, if they're not being moved that's fine but let's keep the option open

@stan-dot
Copy link
Contributor Author

stan-dot commented Aug 1, 2024

still not right, need to talk to the scientists

@stan-dot
Copy link
Contributor Author

stan-dot commented Aug 1, 2024

@DiamondJoseph this fits the Hyperion model of 'the changes fit our model of the beamline', and it passes the tests, can we merge this now?

@stan-dot
Copy link
Contributor Author

stan-dot commented Aug 1, 2024

dodal connect i22
Attempting connection to i22 (using dodal.beamlines.i22)
16 devices connected :
        oav
        fswitch
        slits_6
        slits_5
        slits_4
        slits_3
        slits_2
        slits_1
        undulator
        hfm
        vfm
        it
        i0
        waxs
        synchrotron
        saxs

this was successful, ready to merge

@stan-dot stan-dot force-pushed the 587-create-device-for-i22-slits_4 branch from 434a7e7 to fd1a91c Compare August 8, 2024 09:05
Comment on lines 29 to 30
self.horizontal_dso = Motor(prefix + "HDSO")
self.vertical_dso = Motor(prefix + "VDSO")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should: As someone not involved in i22, it's not clear what dso means here. Could we have a comment or rename the variable to something more human readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not either. that is how it was defined in EPICS. I did inquire on 07.08.2024 and received no response since.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, well I don't feel comfortable us having code in dodal where no-one understands what it means. Could you please chase up? Have you asked Scientists+Controls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed up again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@DominicOram

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So they stand for defining_slit_opening? Can we just use gap as that's what we use everywhere else

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also assuming that "defining slit opening" -> "slit width" -> "gap"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think they are all equivalent, we use gap everywhere else, it's succinct (unlike defining_slit_opening) and makes more sense for x and y (unlike width which sort of implies just x). So I think we should stick to gap at the ophyd level

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I change it into x_gap etc, and moving the 'dso' into the class docstring

@stan-dot stan-dot force-pushed the 587-create-device-for-i22-slits_4 branch from fd1a91c to 7ca332f Compare August 16, 2024 10:02
@DiamondJoseph
Copy link
Contributor

class Slits(StandardReadable):
    def __init__(self, prefix: str, name: str = "") -> None:
        with self.add_children_as_readables():
            self.x_gap = Motor(prefix + "X:SIZE")
            self.y_gap = Motor(prefix + "Y:SIZE")
            self.x_centre = Motor(prefix + "X:CENTRE")
            self.y_centre = Motor(prefix + "Y:CENTRE")
        super().__init__(name)


class SpecialSlits(StandardReadable):
    def __init__(self, prefix: str, name: str = "") -> None:
        with self.add_children_as_readables():
            self.x = Motor(prefix + "X")
            self.y = Motor(prefix + "Y")
            self.x_gap = Motor(prefix + "HDSO")
            self.y_gap = Motor(prefix + "VDSO")
        super().__init__(name)

At which point we've got some very similar looking classes. It'd be nice if we could a. confirm that SpecialSlits.x is defining x centre, not e.g. x_min, and b. treat them all as some sort of generic slit with x_centre, x_gap, y_centre, y_gap

@stan-dot
Copy link
Contributor Author

yes, it is centre. we can make both classes have the same properties but they do need to have different logic.

@DominicOram and @DiamondJoseph, how would you like this to be implemented?
the ways I see

  • (default) keep 2 classes
  • just 1 class with an override option for the channel names - that done either with a ternary inline logic or a big logic branch inside add_children_as_readables
  • some interface that both classes implement that defines the same 4 properties

@stan-dot stan-dot force-pushed the 587-create-device-for-i22-slits_4 branch from 5b138a3 to 291050c Compare August 16, 2024 15:07
@DiamondJoseph
Copy link
Contributor

* just 1 class with an override option for the channel names (in the constructor with the 5/6 case as the default and the 1/6 case as the special override)

would be my preference.

@DominicOram
Copy link
Contributor

My preference would be:

  1. Create an alias at the EPICs layer so that the PVs are the same - we have done this for a set on i03
  2. If we're short on time ask for the alias and do the override of channel names

@stan-dot
Copy link
Contributor Author

closing as this will be solved at the epics layer

@stan-dot stan-dot closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create device for i22 slits_4
4 participants